Add specification property field's values restrictions based on validator setting#3329
Conversation
penja
changed the title
penja
force-pushed
the
property_shema_validation_restrictions
branch
from
d912241 to
75d8123
Compare
penja
changed the title
| } | ||
|
|
||
| return $propertyMetadata->withIri($iri)->withRequired($required ?? false); | ||
| $propertyMetadata = $propertyMetadata->withIri($iri)->withRequired($required ?? false); |
There was a problem hiding this comment.
We don't break the above loop anymore, and it looks like we can have multiple constraints, hence different iris? I'm not sure that this case is handled / implemented correctly.
There was a problem hiding this comment.
@soyuka Like in previous implementation we continue to set IRI and required only if it was not set before.
| $schema = []; | ||
| } | ||
|
|
||
| $schema += array_merge(...$restrictions); |
There was a problem hiding this comment.
Do we really need this spread operator / array_merge?
There was a problem hiding this comment.
Yes we must merge all restrictions into one schema
dunglas
left a comment
dunglas
left a comment
There was a problem hiding this comment.
Great addition! Thanks for working on this.
| private $decorated; | ||
| private $validatorMetadataFactory; | ||
| /** | ||
| * @var iterable<PropertySchemaRestrictionMetadataInterface> |
There was a problem hiding this comment.
Can you move it to the constructor please?
| public function __construct(ValidatorMetadataFactoryInterface $validatorMetadataFactory, PropertyMetadataFactoryInterface $decorated) | ||
| { | ||
| public function __construct( | ||
| ValidatorMetadataFactoryInterface $validatorMetadataFactory, |
There was a problem hiding this comment.
We put every parameters on the same line (same everywhere).
| public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null, bool $initializable = null, $default = null, $example = null) | ||
| { | ||
| /** | ||
| * @var array |
There was a problem hiding this comment.
to remove (and should be array|null)
| /** | ||
| * @param PropertySchemaRestrictionMetadataInterface[] $restrictionsMetadata | ||
| */ | ||
| public function __construct(ValidatorMetadataFactoryInterface $validatorMetadataFactory, PropertyMetadataFactoryInterface $decorated, iterable $restrictionsMetadata) |
There was a problem hiding this comment.
Can we default iterable $restrictionsMetadata to an array? I fear it may break some implementations if we leave this as-is.
soyuka
left a comment
soyuka
left a comment
There was a problem hiding this comment.
nice thanks, may you rebase/sqash? I'd like to merge this.
penja
force-pushed
the
property_shema_validation_restrictions
branch
from
0a7461a to
e9bdc58
Compare
|
@soyuka Thanks, done. |
soyuka
force-pushed
the
property_shema_validation_restrictions
branch
from
e9bdc58 to
38d6955
Compare
|
Thanks @penja! |
| case Type::BUILTIN_TYPE_INT: | ||
| case Type::BUILTIN_TYPE_FLOAT: | ||
| if (isset($constraint->min)) { | ||
| $restriction['minimum'] = (int) $constraint->min; | ||
| } | ||
|
|
||
| if (isset($constraint->max)) { | ||
| $restriction['maximum'] = (int) $constraint->max; | ||
| } |
There was a problem hiding this comment.
This part looks wrong. Length is a string constraint. There seems to be confusion with Range, implemented in #4158
There was a problem hiding this comment.
That's right. Could you provide a PR to fix it?
There was a problem hiding this comment.
PS: thanks @penja for implementing this feature!
5 participants
Generate specification property restrictions based on Symfony’s built-in validator.